Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assembler: Allow to zoom out the preview #83034

Merged
merged 15 commits into from
Oct 24, 2023

Conversation

arthur791004
Copy link
Contributor

@arthur791004 arthur791004 commented Oct 16, 2023

Related to #82296

Proposed Changes

  • Add the zoom-out toolbar to the DeviceSwitcher
  • Use the useZoomOut hook to control the zoom-out behavior
assembler-zoom-out.mov

Figma: Tv3pYqA3EcRfiXC31IxrXE-fi-2092%3A14696

Testing Instructions

  • Go to the Theme Showcase
  • Scroll down to select the Assembler CTA
  • On the Assembler screen
    • Pick patterns
    • Click the slider on the top-right corner to test the zoom-out behavior

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • https://wpcalypso.wordpress.com/devdocs/docs/testing/index.md for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

@github-actions
Copy link

github-actions bot commented Oct 16, 2023

@arthur791004 arthur791004 self-assigned this Oct 16, 2023
@@ -85,7 +84,6 @@ const PatternLargePreview = ( {
>
{ viewportHeight && (
<PatternRenderer
key={ device }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miksansegundo Do you remember why we use the device as a key to force rendering?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was introduced in https://github.com/Automattic/wp-calypso/pull/76132/files#r1175185900.

I remember this is for the case of patterns like Link In Bio with min-height: 100vh because when the device viewport changes the patterns must be re-rendered to calculate their content height.

However, we don't have Link In Bio patterns in the assembler, and that logic was cleaned recently in #81310

We can probably remove it. Let me test it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with min-height: 100vh and the scrolling is gone thanks to the refactoring.

Screen.Recording.2566-10-18.at.10.22.30.mov

We could remove that key but then the animation won't look good. See this Intro pattern dancing 😆 when switching viewports.

Screen.Recording.2566-10-18.at.10.23.27.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's dancing 😆 But the current behavior is not good either as all patterns become blank due to the key changes. Anyway, I'll revert the change and we can improve the experience later

@matticbot
Copy link
Contributor

matticbot commented Oct 16, 2023

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~299 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
entry-stepper              +2999 B  (+0.1%)     +299 B  (+0.1%)
entry-main                 +2846 B  (+0.2%)     +303 B  (+0.1%)
entry-login                 +756 B  (+0.1%)     +114 B  (+0.0%)
entry-subscriptions         +681 B  (+0.0%)     +109 B  (+0.0%)
entry-domains-landing       +681 B  (+0.1%)     +109 B  (+0.1%)
entry-browsehappy           +681 B  (+0.5%)     +109 B  (+0.3%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~19823 bytes added 📈 [gzipped])

name                       parsed_size            gzip_size
theme                         +72783 B  (+11.5%)   +22566 B  (+11.9%)
import-flow                    +3130 B   (+0.1%)    +1690 B   (+0.2%)
pattern-assembler-step         +3040 B   (+0.2%)    +1738 B   (+0.4%)
update-design-flow             +1202 B   (+0.1%)     -570 B   (-0.2%)
free-flow                      +1200 B   (+0.1%)     +260 B   (+0.1%)
link-in-bio-tld-flow           +1130 B   (+0.1%)     -874 B   (-0.2%)
videopress-flow                 +973 B   (+0.1%)      +56 B   (+0.0%)
write-flow                      +880 B   (+0.1%)     -250 B   (-0.1%)
build-flow                      +880 B   (+0.1%)     -250 B   (-0.1%)
signup                          +840 B   (+0.3%)      +14 B   (+0.0%)
jetpack-connect                 +840 B   (+0.1%)      +14 B   (+0.0%)
accept-invite                   +840 B   (+0.2%)      +14 B   (+0.0%)
home                            +777 B   (+0.1%)      +46 B   (+0.0%)
security                        -649 B   (-0.1%)     -146 B   (-0.1%)
devdocs                         +420 B   (+0.2%)      +65 B   (+0.1%)
settings                        +264 B   (+0.0%)     +659 B   (+0.3%)
site-setup-flow                 +262 B   (+0.0%)     -201 B   (-0.0%)
comments                        -218 B   (-0.0%)     -556 B   (-0.3%)
settings-podcast                +208 B   (+0.0%)     +129 B   (+0.1%)
posts-custom                    +208 B   (+0.0%)     +410 B   (+0.2%)
posts                           +208 B   (+0.0%)     +410 B   (+0.2%)
plans                           +205 B   (+0.0%)    +1311 B   (+0.3%)
start-writing-flow              +155 B   (+0.5%)       +4 B   (+0.0%)
design-first-flow               +155 B   (+0.5%)       +6 B   (+0.1%)
stats                           +154 B   (+0.0%)     -960 B   (-0.5%)
new-hosted-site-flow            +150 B   (+0.9%)       +5 B   (+0.1%)
with-theme-assembler-flow       +140 B   (+0.2%)      +15 B   (+0.1%)
people                          -140 B   (-0.0%)     -685 B   (-0.4%)
ai-assembler-flow               +140 B   (+0.2%)      +20 B   (+0.1%)
plugins                         +137 B   (+0.0%)     +672 B   (+0.1%)
plugin-bundle-flow              +137 B   (+0.1%)     +672 B   (+1.3%)
settings-newsletter             -115 B   (-0.0%)     -699 B   (-0.6%)
newsletter-flow                 +115 B   (+0.7%)       +9 B   (+0.2%)
link-in-bio-flow                +115 B   (+0.8%)      +11 B   (+0.3%)
gutenberg-editor                +115 B   (+0.0%)     +110 B   (+0.1%)
export                          -103 B   (-0.0%)      -36 B   (-0.0%)
checkout                         +91 B   (+0.0%)     +504 B   (+0.1%)
connect-domain                   +75 B   (+0.3%)       +3 B   (+0.0%)
media                            +74 B   (+0.0%)     +839 B   (+0.2%)
settings-writing                 +71 B   (+0.0%)    +1045 B   (+0.7%)
import-hosted-site-flow          -59 B   (-0.0%)     -455 B   (-0.1%)
site-purchases                   +44 B   (+0.0%)     +275 B   (+0.1%)
purchases                        +44 B   (+0.0%)      -65 B   (-0.0%)
domains                          +44 B   (+0.0%)     +288 B   (+0.1%)
account                          +44 B   (+0.0%)     +293 B   (+0.1%)
reader                           +40 B   (+0.0%)      -40 B   (-0.0%)
customize                        -37 B   (-0.0%)     -920 B   (-1.2%)
jetpack-app                      +34 B   (+0.0%)     +637 B   (+0.5%)
marketplace                      -25 B   (-0.0%)     -206 B   (-0.1%)
preview                          -24 B   (-0.0%)       -6 B   (-0.0%)
themes                           -23 B   (-0.0%)     -308 B   (-0.1%)
settings-security                +13 B   (+0.0%)      +25 B   (+0.0%)
settings-discussion              +13 B   (+0.0%)      +21 B   (+0.0%)
pages                            -11 B   (-0.0%)      +86 B   (+0.1%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~7004 bytes added 📈 [gzipped])

name                                                                         parsed_size           gzip_size
async-load-automattic-global-styles-src-components-global-styles-variations     -71752 B  (-4.9%)   -20661 B  (-4.9%)
async-load-automattic-design-preview                                              +729 B  (+0.1%)    +2350 B  (+0.6%)
async-load-design-blocks                                                          +346 B  (+0.0%)    +1352 B  (+0.2%)
async-load-store-app-store-stats                                                  +196 B  (+0.1%)     +723 B  (+1.2%)
async-load-signup-steps-woocommerce-install-step-business-info                    +137 B  (+0.2%)     +672 B  (+3.1%)
async-load-signup-steps-plans-theme-preselected                                   +137 B  (+0.0%)     +672 B  (+0.5%)
async-load-signup-steps-plans                                                     +137 B  (+0.0%)     +672 B  (+0.5%)
async-load-quick-language-switcher                                                +137 B  (+0.2%)     +672 B  (+2.6%)
async-load-calypso-lib-account-settings-helper                                    +137 B  (+0.1%)     +672 B  (+1.2%)
async-load-design-wordpress-components-gallery                                    +125 B  (+0.0%)    +1375 B  (+0.8%)
async-load-calypso-my-sites-checkout-modal                                        -115 B  (-0.0%)     -476 B  (-0.1%)
async-load-calypso-blocks-support-article-dialog-dialog                            +81 B  (+0.0%)     +141 B  (+0.2%)
async-load-calypso-blocks-support-article-dialog                                   +81 B  (+2.5%)      +39 B  (+3.2%)
async-load-calypso-post-editor-media-modal                                         +73 B  (+0.0%)     +599 B  (+0.2%)
async-load-automattic-help-center                                                  +68 B  (+0.0%)     +374 B  (+0.3%)
async-load-publish                                                                 +40 B  (+0.1%)      +15 B  (+0.1%)
async-load-calypso-my-sites-stats-stats-post-detail                                +40 B  (+0.1%)      +15 B  (+0.1%)
async-load-calypso-my-sites-site-settings-seo-settings-form                        +40 B  (+0.0%)       +9 B  (+0.0%)
async-load-calypso-components-web-preview-component                                +40 B  (+0.0%)     -264 B  (-0.1%)
async-load-design-playground                                                       +34 B  (+0.0%)     +646 B  (+0.2%)
async-load-calypso-post-editor-editor-media-modal                                  +34 B  (+0.0%)     +631 B  (+0.2%)
async-load-design                                                                  +32 B  (+0.0%)     +460 B  (+0.1%)
async-load-signup-steps-design-picker                                              +13 B  (+0.0%)      +13 B  (+0.1%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@lucasmendes-design
Copy link

lucasmendes-design commented Oct 16, 2023

It's amazing @arthur791004 🤩 I didn't know that you would follow this direction. Very nice iteration. I'll share just some details to make this component amazing.

Figma file: Tv3pYqA3EcRfiXC31IxrXE-fi-2469:34894#585639702

image

Hover state:

Hover.effect.mov

Focus state:

Screen.Recording.2023-10-16.at.10.58.57.mov

Copy link
Member

@okmttdhr okmttdhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

badge
I noticed if we zoom out when scrolling enough, it appears as if nothing is in the preview.

Screen.Recording.2023-10-17.at.13.54.58.mov

Comment on lines 106 to 116
color="#C3C4C7"
trackColor="#50575E"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

badge

It is out of the scope of this PR, but it'd be nice if we could use the Calypso color scheme valuables in JS file.

@arthur791004
Copy link
Contributor Author

I noticed if we zoom out when scrolling enough, it appears as if nothing is in the preview.

Yes, I noticed this issue but I'm considering how to resolve it. Sometimes the layout re-render immediately so the scrollbar will disappear. However, sometimes it doesn't work, and the scrollbar will be there for a while 🤦‍♂️

@arthur791004
Copy link
Contributor Author

I'll share just some details to make this component amazing.

@lucasmendes-design As the component is from Gutenberg, I'm unsure whether it's possible to change all details but I can try it tomorrow 🙂

Additionally, do you think the zoom-out behavior of the large preview is expected? Or should we resize the large preview with the outer border as well?

@lucasmendes-design
Copy link

As the component is from Gutenberg, I'm unsure whether it's possible to change all details but I can try it tomorrow 🙂

All good. I tried to better align with the viewport component (Desktop, tablet, and mobile). Where the hover is darker and the line is thinner.
image

Additionally, do you think the zoom-out behavior of the large preview is expected? Or should we resize the large preview with the outer border as well?

Not sure if I understood correctly. What do you mean by large preview? Without the drawer?

@taipeicoder
Copy link
Contributor

The empty state looks a bit odd!
Screenshot 2023-10-18 at 10 49 51 AM

@taipeicoder
Copy link
Contributor

@arthur791004 @lucasmendes-design WDYT about setting a min (say, 25%) to the slider? I don't see much value allowing users to zoom all this way 😅:
Screenshot 2023-10-18 at 10 51 20 AM

@miksansegundo
Copy link
Contributor

Would it be possible to calculate programmatically the zoom value required to show all the patterns on the preview?

It would be useful to have this value as a zoom preset so we can adjust the zoom automatically for users when selecting styles.

Something like what I'm doing in this video to adjust the zoom but do it automatically rather than manually.

Screen.Recording.2566-10-18.at.11.26.03.mov

@arthur791004
Copy link
Contributor Author

arthur791004 commented Oct 18, 2023

Not sure if I understood correctly. What do you mean by large preview? Without the drawer?

@lucasmendes-design Sorry for not being clear 🙈 Here are examples

Zoom out the content of the preview Zoom out the entire preview
Screenshot 2023-10-18 at 4 06 19 PM Screenshot 2023-10-18 at 4 08 03 PM

But I just noticed #82296 (comment) so I think the answer is to zoom out the entire preview including the gray border 😆 But the downside is the scrolling area would be limited to the zoomed-out preview (i.e. user can scroll the preview inside the gray border area).

@fushar
Copy link
Contributor

fushar commented Oct 18, 2023

Would it be possible to calculate programmatically the zoom value required to show all the patterns on the preview?

I love this. If this is possible, let's make the value as the minimum possible zoom-out value. Currently, it looks off if you can zoom out to the extent that the patterns are not legible at all 😅 :

image

Edit: sorry, just saw @taipeicoder 's comment above!

@arthur791004 @lucasmendes-design WDYT about setting a min (say, 25%) to the slider? I don't see much value allowing users to zoom all this way 😅:

@arthur791004 arthur791004 force-pushed the feat/assembler-zoom-out-preview branch from f9eb541 to efc2673 Compare October 18, 2023 09:17
@arthur791004
Copy link
Contributor Author

I made changes below

Here is the latest demo:

Screen.Recording.2023-10-18.at.6.17.36.PM.mov

@lucasmendes-design
Copy link

lucasmendes-design commented Oct 18, 2023

It looks amazing team 🚀

Some details that I think it's worth it to check:

Alignment

The viewport component is not aligned in the center of the site preview.
image

Empty state

When we don't have any pattern. I'm not sure if we need to allow the zoom-out. It's strange and the font is tiny.
image

Avoid tiny action bar

We need to think more about this one here. I'm pretty sure we'll receive some negative feedback regarding the size of the buttons. Image below.
image

Proposed solution
Keep the same size of the action bar but instead of placing it inside the patterns we can place it outside the site preview.

  • Between 100% and +-30%. We show the action bar inside the pattern.
  • Below 30% we show the action bar outside the pattern.

Something around that. WDYT?
Group 1

@lucasmendes-design
Copy link

Would it be possible to calculate programmatically the zoom value required to show all the patterns on the preview?

I love this. If this is possible, let's make the value as the minimum possible zoom-out value. Currently, it looks off if you can zoom out to the extent that the patterns are not legible at all 😅

+1000.

@lucasmendes-design
Copy link

Another solution could be to hide the action bar when the site preview is too small and keep the same size as for, let's say, 100% - 50%. Let me know your thoughts for this and what it's possible here :)

@arthur791004
Copy link
Contributor Author

The viewport component is not aligned in the center of the site preview.

Yes, but it's intended as we want to avoid the zoom-out slider overlapping the viewport component on the smaller screen. Do you have any idea on this 🤔

When we don't have any pattern. I'm not sure if we need to allow the zoom-out. It's strange and the font is tiny.

Yes, I think we can disable it if there is no selected pattern 🙂

We need to think more about this one here. I'm pretty sure we'll receive some negative feedback regarding the size of the buttons. Image below.

We discussed this on our sync but we assume people might not try to use the action bar when the resolution is too small. We can investigate how to show the action bar outside the pattern as it's the current behavior of the Editor

@arthur791004
Copy link
Contributor Author

@lucasmendes-design I made changes to keep the size of the action bar the same during zoom-out and changed the position to always stick on the top of the active pattern. WDYT?

Screen.Recording.2023-10-19.at.9.49.22.PM.mov

@arthur791004 arthur791004 force-pushed the feat/assembler-zoom-out-preview branch from 3275eb0 to 36ff2ec Compare October 19, 2023 13:59
@lucasmendes-design
Copy link

Yes, but it's intended as we want to avoid the zoom-out slider overlapping the viewport component on the smaller screen. Do you have any idea on this 🤔

Understood. It seems wrong the way it is, we're optimizing for the edge case instead of the 80/90% of users. I think we could consider a better solution for a small screen or when it has a specific screen size.

I made changes to keep the size of the action bar the same during zoom-out and changed the position to always stick on the top of the active pattern. WDYT?

We discussed this on our sync but we assume people might not try to use the action bar when the resolution is too small. We can investigate how to show the action bar outside the pattern as it's the current behavior of the Editor

I would try to have something simpler. The action bar above the patterns is not clear to me =/

Suggestion:

  1. Keep the action bar the same size inside the patterns.
  2. If users zoom out more than 40% (I believe this number will be more like a try-and-error approach). We can hide the action bar.

WDYT?

@arthur791004
Copy link
Contributor Author

Understood. It seems wrong the way it is, we're optimizing for the edge case instead of the 80/90% of users. I think we could consider a better solution for a small screen or when it has a specific screen size.

Okay, I'll keep the viewport component aligned in the center of the site preview, and think about how to deal with the small screen.

The action bar above the patterns is not clear to me

But it's the current behavior of the Editor for a long time 😂

Suggestion:

  1. Keep the action bar the same size inside the patterns.
  2. If users zoom out more than 40% (I believe this number will be more like a try-and-error approach). We can hide the action bar.

If we keep the action bar inside the patterns, it might look a little weird for the patterns with smaller height

0.6x 0.4x
image image

Sticking to the top seems to be better. We can keep the action bar visible and it aligns with the behavior of the Editor so the user might not feel confused about the action bar when proceeding to the Editor.

image

@lucasmendes-design
Copy link

But it's the current behavior of the Editor for a long time 😂

Yep, but there are some nuances:

  • The Editor doesn't zoom out.
  • You must click on the patterns to show the action bar.
  • The action bar is persistent. Still visible even if I hover over other components or scroll the page.

In the assembler, we have the hover as our main interaction to show the action bar. So, if you hover and show the action bar outside the pattern, there is just one path to access that with the mouse (Red line in the image below).

image

Sticking to the top seems to be better. We can keep the action bar visible and it aligns with the behavior of the Editor so the user might not feel confused about the action bar when proceeding to the Editor.

+1. This is a good point. I would use this behavior only when users zoom out. Let's say, more or less, 0.6x.

@arthur791004
Copy link
Contributor Author

The Editor doesn't zoom out.

Yes, but the behavior of the style variation preview is quite similar

Screenshot 2023-10-20 at 9 49 01 AM

You must click on the patterns to show the action bar.
The action bar is persistent. Still visible even if I hover over other components or scroll the page.

Yes, I agree. These are the most different parts.

@arthur791004
Copy link
Contributor Author

@lucasmendes-design I made the following changes

  • Centralize the viewport component
  • Stick the action bar outside the pattern only when the zoom-out ratio is smaller than 0.75

WDYT?

Screen.Recording.2023-10-20.at.3.46.06.PM.mov

@lucasmendes-design
Copy link

It's perfect Arthur. Good iteration here 💯 Thanks for that :)

@lucasmendes-design
Copy link

Yes, but the behavior of the style variation preview is quite similar

Ops haha. I thought this screenshot was not a thing yet. Thanks for sharing :) So, I think our component is pretty much aligned with the Editor.

@taipeicoder
Copy link
Contributor

Non-blocking. There's an odd behavior with scaling carrying over across device switches. WDYT about resetting zoom whenever the user switches devices? cc: @lucasmendes-design

Screen.Capture.on.2023-10-23.at.07-49-08.mp4

@autumnfjeld
Copy link
Contributor

WDYT about setting a min (say, 25%) to the slider? I don't see much value allowing users to zoom all this way 😅:

Yes agree, the left most point on the slide bar should be a reasonable value, as @taipeicoder suggested.

@arthur791004
Copy link
Contributor Author

Non-blocking. There's an odd behavior with scaling carrying over across device switches. WDYT about resetting zoom whenever the user switches devices?

Screen.Capture.on.2023-10-23.at.07-49-08.mp4

Nice catch! Addressed this issue via 33b7e4f

Copy link
Contributor

@fushar fushar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUPER SMOOTH ❤️

const [ patternLargePreviewStyle, setPatternLargePreviewStyle ] = useState( {
'--pattern-large-preview-background': backgroundColor,
} as CSSProperties );
const patternLargePreviewStyle = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit: I think useMemo() here is unnecessary as patternLargePreviewStyle is not props / dependency of anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the style prop of the ul element 😆

@lucasmendes-design
Copy link

WDYT about resetting zoom whenever the user switches devices?

+1. Good idea. We could add that.

@arthur791004 arthur791004 merged commit 430e0b0 into trunk Oct 24, 2023
3 checks passed
@arthur791004 arthur791004 deleted the feat/assembler-zoom-out-preview branch October 24, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants